newton/tests/test_speculative_contacts.py (1)

138-165: Please add a linear GJK regression alongside the angular one.

The suite covers linear motion only on sphere-based paths and covers boxes only with angular motion. A simple approaching/diverging box-box case would exercise the remaining non-primitive speculative acceptance branch and would catch regressions in the directed write_contact() path much earlier.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_speculative_contacts.py` around lines 138 - 165, Add a
second test inside (or next to) test_speculative_angular_velocity that performs
a linear GJK regression for box-box speculative contacts: reuse the
newton.ModelBuilder setup (builder, builder.rigid_gap = 0.0), create two box
bodies (similar to body_a and body_b) positioned apart on the x-axis, set their
body_qd linear velocity components so they are approaching each other (e.g.,
nonzero vx on body_a or opposite vx on body_b) while keeping angular rates zero,
finalize the model and state, create the same newton.SpeculativeContactConfig
and newton.CollisionPipeline (broad_phase="nxn", speculative_config=config),
call pipeline.collide(state, contacts) and assert
contacts.rigid_contact_count.numpy()[0] > 0 to validate linear speculative
acceptance; name the test to indicate linear GJK regression (e.g.,
test_speculative_linear_velocity) so it exercises the non-primitive speculative
acceptance branch and directed write_contact path.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

newton/_src/geometry/narrow_phase.py (1)

1768-1772: Consider caching the empty speculative arrays.

These empty arrays are allocated on every launch_custom_write call. Since NarrowPhase already caches similar sentinel buffers (e.g., _empty_edge_indices, _empty_edge_range), the speculative fallback arrays could be cached in __init__ to avoid repeated allocations.

♻️ Suggested caching in __init__

 # In __init__, after allocating other sentinel buffers:
+            # Sentinel arrays for speculative fallback (when not provided)
+            self._empty_lin_vel = wp.empty(0, dtype=wp.vec3, device=device)
+            self._empty_ang_speed = wp.empty(0, dtype=wp.float32, device=device)

 # In launch_custom_write:
-        _empty_vec3 = wp.empty(0, dtype=wp.vec3, device=device)
-        _empty_float = wp.empty(0, dtype=wp.float32, device=device)
-        _slv = shape_lin_vel if shape_lin_vel is not None else _empty_vec3
-        _sasb = shape_ang_speed_bound if shape_ang_speed_bound is not None else _empty_float
+        _slv = shape_lin_vel if shape_lin_vel is not None else self._empty_lin_vel
+        _sasb = shape_ang_speed_bound if shape_ang_speed_bound is not None else self._empty_ang_speed

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/narrow_phase.py` around lines 1768 - 1772, Create and
cache the empty speculative arrays in NarrowPhase.__init__ (e.g.,
self._empty_spec_vec3 = wp.empty(0, dtype=wp.vec3, device=self.device) and
self._empty_spec_float = wp.empty(0, dtype=wp.float32, device=self.device) using
the same device as the instance), then update launch_custom_write to use those
cached sentinels instead of allocating new ones: replace local allocations of
_empty_vec3/_empty_float with the cached attributes and keep the same fallback
logic for _slv (shape_lin_vel) and _sasb (shape_ang_speed_bound). Ensure the
cached arrays match dtype and device so existing code that references _slv/_sasb
continues to work.

newton/_src/geometry/narrow_phase.py (2)

880-894: ⚠️ Potential issue | 🟠 Major

Speculative heightfield pairs still use the non-speculative midphase.

This branch still calls heightfield_vs_convex_midphase() with only shape_gap, so the speculative extension never widens the heightfield cell query. Fast convexs can still cross into a new heightfield cell between frames without emitting the triangle pairs needed for the later GJK/MPR pass.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/narrow_phase.py` around lines 880 - 894, The heightfield
branch is calling heightfield_vs_convex_midphase with only shape_gap, so
speculative extension isn't applied; change the call to pass the
speculative-widened gap (e.g., shape_gap + shape_speculative_extension or a
dedicated shape_speculative_gap) and update heightfield_vs_convex_midphase's
signature/uses to accept and apply that speculative gap when building the
heightfield cell query; keep the j!=0 early-continue logic and locate this
change around GeoType.HFIELD, heightfield_data, shape_heightfield_index and the
heightfield_vs_convex_midphase invocation.

---

1835-1862: ⚠️ Potential issue | 🟠 Major

NarrowPhase.launch() still can't drive speculative contacts.

launch_custom_write() now accepts shape_lin_vel, shape_ang_speed_bound, speculative_dt, and max_speculative_extension, but this wrapper neither exposes nor forwards them. Any caller that still uses launch() has no path to enable the new feature.

🧩 Suggested fix

     def launch(
         self,
         *,
@@
         contact_penetration: wp.array[float],  # negative if bodies overlap
         contact_count: wp.array[int],  # Number of active contacts after narrow
         contact_tangent: wp.array[wp.vec3] | None = None,  # Represents x axis of local contact frame (None to disable)
+        shape_lin_vel: wp.array[wp.vec3] | None = None,
+        shape_ang_speed_bound: wp.array[wp.float32] | None = None,
+        speculative_dt: float = 0.0,
+        max_speculative_extension: float = 0.0,
         device: Devicelike | None = None,  # Device to launch on
         **kwargs: Any,
     ) -> None:
@@
         self.launch_custom_write(
             candidate_pair=candidate_pair,
             candidate_pair_count=candidate_pair_count,
@@
             mesh_edge_indices=mesh_edge_indices,
             shape_edge_range=shape_edge_range,
             writer_data=writer_data,
+            shape_lin_vel=shape_lin_vel,
+            shape_ang_speed_bound=shape_ang_speed_bound,
+            speculative_dt=speculative_dt,
+            max_speculative_extension=max_speculative_extension,
             device=device,
         )

Also applies to: 2312-2448

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/narrow_phase.py` around lines 1835 - 1862, The
NarrowPhase.launch() wrapper does not expose or forward the new
speculative-contact parameters to launch_custom_write (shape_lin_vel,
shape_ang_speed_bound, speculative_dt, max_speculative_extension), so callers
cannot enable speculative contacts; update the NarrowPhase.launch() signature to
accept those four parameters (with same defaults) and pass them through when
calling launch_custom_write, and repeat the same change for the other wrapper
overload around the later block (the one covering lines ~2312-2448) so both
wrappers expose and forward shape_lin_vel, shape_ang_speed_bound,
speculative_dt, and max_speculative_extension to launch_custom_write.

newton/_src/sim/collide.py (2)

928-935: ⚠️ Potential issue | 🟠 Major

Deterministic mode still enables for hydroelastic scenes.

The docstring warns that hydroelastic contacts are not yet covered, but this branch still enables deterministic sorting for them — and in hydroelastic write paths multiple contacts per shape pair share the same sort key, so their relative order remains launch-dependent and results in nondeterministic rigid_contact_* buffers. Additionally, since enabling contact_matching now force-sets deterministic=True, a user could unintentionally activate this path on a hydroelastic scene just by opting into matching.

Consider rejecting the combination explicitly:

🛡️ Suggested guard

         self.requires_grad = requires_grad
         self.deterministic = deterministic
         per_contact_props = self.narrow_phase.hydroelastic_sdf is not None
         if deterministic:
+            if per_contact_props:
+                raise ValueError(
+                    "CollisionPipeline(deterministic=True) does not support "
+                    "hydroelastic contacts yet."
+                )
             with wp.ScopedDevice(device):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sim/collide.py` around lines 928 - 935, The deterministic branch
is being enabled for hydroelastic scenes (per_contact_props is True when
self.narrow_phase.hydroelastic_sdf is set), which is invalid because
hydroelastic contacts produce multiple contacts per pair that keep
launch-dependent ordering; update the guard in the constructor around the
deterministic/contact-sorting logic (the block creating self._sort_key_array and
self._contact_sorter/ContactSorter) to explicitly reject or disable
deterministic mode when self.narrow_phase.hydroelastic_sdf is present (and also
when contact_matching forces deterministic=True), e.g. raise a clear exception
or fallback to deterministic=False if per_contact_props is True; reference the
deterministic flag, per_contact_props, self.narrow_phase.hydroelastic_sdf,
ContactSorter and the code path that force-sets deterministic via
contact_matching to locate and adjust the logic.

---

767-810: ⚠️ Potential issue | 🟠 Major

Speculative setting mismatch still not validated in the expert-components branch.

When a caller supplies a prebuilt narrow_phase, the constructor only threads speculative=self._speculative_enabled through in the non-expert path (line 894). In the expert branch this check is skipped: if the passed-in narrow_phase was built with speculative=False but speculative_config is not None, speculative expansion in write_contact/AABBs silently no-ops; if it was built with speculative=True but speculative_config is None, collide() still passes empty shape_lin_vel/shape_ang_speed_bound arrays, and any kernel compiled to index them will OOB.

Please validate the supplied narrow phase’s speculative setting against self._speculative_enabled here, before the sanity checks on max_candidate_pairs:

🛡️ Suggested guard

             if deterministic and not narrow_phase.deterministic:
                 raise ValueError(
                     "CollisionPipeline(deterministic=True) requires a deterministic "
                     "NarrowPhase. Either omit narrow_phase or construct it with "
                     "deterministic=True."
                 )
+            if getattr(narrow_phase, "speculative", False) != self._speculative_enabled:
+                raise ValueError(
+                    "speculative_config must match the supplied NarrowPhase.speculative "
+                    f"setting (config enabled={self._speculative_enabled}, "
+                    f"narrow_phase.speculative={getattr(narrow_phase, 'speculative', False)})"
+                )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sim/collide.py` around lines 767 - 810, The expert-component
branch fails to validate that the supplied NarrowPhase's speculative setting
matches the pipeline's speculative enablement; before the max_candidate_pairs
check, assert that the provided narrow_phase has a boolean attribute speculative
and that narrow_phase.speculative == self._speculative_enabled, and if not raise
a ValueError describing the mismatch (e.g., ask to pass a narrow_phase built
with speculative matching CollisionPipeline(_speculative_enabled) or adjust
speculative_config); reference the symbols narrow_phase,
self._speculative_enabled, and self.narrow_phase and perform this check prior to
the existing narrow_phase.max_candidate_pairs validation.

newton/_src/sim/collide.py (2)

1067-1075: Validate the per-call dt override before using it.

speculative_dt = dt if dt is not None else cfg.collision_update_dt executes before validation, so a negative/NaN caller-supplied dt is already bound to speculative_dt by the time the check runs. The ValueError still fires before any kernel launch so this is not an active bug, but flipping the order makes the intent clearer and prevents future refactors from accidentally using the unchecked value:

🧹 Suggested reorder

-        if self._speculative_enabled:
-            cfg = self.speculative_config
-            speculative_dt = dt if dt is not None else cfg.collision_update_dt
-            if dt is not None:
-                _validate_speculative_scalar(dt, "dt")
-            max_speculative_extension = cfg.max_speculative_extension
+        if self._speculative_enabled:
+            cfg = self.speculative_config
+            if dt is not None:
+                _validate_speculative_scalar(dt, "dt")
+            speculative_dt = dt if dt is not None else cfg.collision_update_dt
+            max_speculative_extension = cfg.max_speculative_extension

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sim/collide.py` around lines 1067 - 1075, The per-call dt should
be validated before being assigned to speculative_dt; move the
_validate_speculative_scalar(dt, "dt") check to occur when dt is not None and
before computing speculative_dt in the block that checks
self._speculative_enabled (using speculative_config, speculative_dt, and
max_speculative_extension) so that any caller-supplied dt is validated first and
only then used to set speculative_dt (falling back to cfg.collision_update_dt
when dt is None).

---

319-328: Duplicate speculative AABB expansion block.

The 10-line directed+angular speculative expansion is inlined twice (has_local_aabb branch and support-function branch). Factoring it into a small @wp.func helper would make both branches easier to keep in sync if bounds math evolves:

♻️ Sketch

`@wp.func`
def _apply_speculative_extension(
    lo: wp.vec3, hi: wp.vec3,
    v: wp.vec3, ang_ext: float,
    speculative_dt: float, cap_scalar: float,
) -> tuple[wp.vec3, wp.vec3]:
    vel_dt = v * speculative_dt
    ang_vec = wp.vec3(ang_ext, ang_ext, ang_ext)
    cap = wp.vec3(cap_scalar, cap_scalar, cap_scalar)
    ext_neg = wp.max(-vel_dt, wp.vec3(0.0, 0.0, 0.0))
    ext_pos = wp.max(vel_dt, wp.vec3(0.0, 0.0, 0.0))
    return lo - wp.min(ext_neg + ang_vec, cap), hi + wp.min(ext_pos + ang_vec, cap)

Also applies to: 353-362

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sim/collide.py` around lines 319 - 328, There is duplicated
speculative AABB expansion logic in both the has_local_aabb and support-function
branches; extract it into a small `@wp.func` (e.g. _apply_speculative_extension)
that takes lo, hi, v (use shape_lin_vel[shape_id]), ang_ext (use
shape_ang_speed_bound[shape_id] * speculative_dt or pass ang_ext scalar),
speculative_dt and cap_scalar (max_speculative_extension) and returns the new
(lo, hi) after computing vel_dt, ang_vec, cap, ext_neg/ext_pos and applying
wp.min/wp.max as in the sketch; replace the duplicated blocks (the one using
shape_lin_vel/shape_ang_speed_bound and the one in the support-function branch)
to call this helper so both branches share the same logic.

newton/_src/geometry/contact_match.py (2)

281-288: Tentative match_index is written even for claim losers.

Pass 1 writes data.match_index[tid] = wp.int32(best_idx) before racing for ownership via atomic_min. Until _resolve_claims_kernel runs, every contender for the same prev[best_idx] holds a transient match_index[tid] == best_idx; only the winner keeps it, the rest are demoted to MATCH_BROKEN in Pass 2. This is correct because both kernels launch on the same stream and the resolve pass runs unconditionally — just worth a one-line comment noting that the intermediate value is only valid after _resolve_claims_kernel completes, to prevent someone from adding an early read of match_index between the two launches.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/contact_match.py` around lines 281 - 288, The current
code writes a tentative match into data.match_index[tid] before racing via
wp.atomic_min on data.prev_claim, so losers will temporarily hold best_idx until
_resolve_claims_kernel runs and demotes them to MATCH_BROKEN; update the block
around match_index assignment (referencing data.match_index, wp.atomic_min,
data.prev_claim, _pack_claim, and _resolve_claims_kernel) to include a concise
one-line comment clarifying that the written match_index is tentative and only
valid after _resolve_claims_kernel completes, warning future readers not to read
match_index between the two kernel launches.

---

828-836: Dummy aliases in the non-sticky path stomp scratch_pos_world across fields.

When sticky=False, every sticky-only slot (src_offset0/1, dst_point0_body, dst_point1_body, dst_offset0_body, dst_offset1_body, dst_normal_sticky) is aliased to the same self._sorter.scratch_pos_world buffer. The kernel is gated by has_sticky==0 so these slots are never read or written, which makes this safe today — but assigning five dst_* outputs to the same array makes static/runtime alias analyzers nervous, and a future refactor that drops (or partially drops) the has_sticky guard would silently produce concurrent writes to one buffer.

Consider either using a single shared dummy alias consistently named (e.g. _dummy_vec3 = wp.empty(1, dtype=wp.vec3) stored on the matcher) or leaving a comment right above this block noting that these aliases are only safe so long as has_sticky gates all reads/writes in _save_sorted_state_kernel.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/contact_match.py` around lines 828 - 836, The dummy
aliases for sticky-only fields in _save_sorted_state_kernel currently all point
to self._sorter.scratch_pos_world which can confuse alias analyzers and risks
future concurrent writes; create a single dedicated dummy buffer on the matcher
(e.g. self._dummy_vec3 = wp.empty(1, dtype=wp.vec3) initialized in the
matcher/constructor) and assign data.src_offset0, data.src_offset1,
data.dst_point0_body, data.dst_point1_body, data.dst_offset0_body,
data.dst_offset1_body, and data.dst_normal_sticky to that _dummy_vec3 when
sticky=False, and keep data.has_sticky = 0 (or alternatively add a clear comment
above this block referencing _save_sorted_state_kernel and the has_sticky guard
if you prefer not to add a buffer).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

newton/tests/test_speculative_contacts.py (2)

117-138: contacts.clear() at line 135 is redundant with the pipeline's own clearing.

Per CollisionPipeline.collide(), the contact count is reset inside compute_shape_aabbs on every call (the generation bump fuses the clear), and an explicit contacts.clear() is only needed in the clear_buffers debug path. Calling it here isn't wrong, but it suggests the test believes the second collide() wouldn't otherwise reset the counter — which it does. Safe to drop, or add a brief comment noting why the explicit clear is kept.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_speculative_contacts.py` around lines 117 - 138, The
explicit call to contacts.clear() in test_speculative_dt_override is redundant
because CollisionPipeline.collide() resets contact buffers (via
compute_shape_aabbs) on each call; remove the contacts.clear() invocation (or if
you prefer to keep it for clarity, replace it with a one-line comment
referencing CollisionPipeline.collide()/compute_shape_aabbs to explain why it’s
unnecessary) so the test does not imply collide() fails to reset the contact
count.

---

467-503: Bare class bodies + module-level test registration — works, but fragile.

TestSpeculativeContacts and TestSpeculativeTunneling have no body (just a docstring), and the test methods are attached at module import time by the for … add_function_test(…) loops below. That means:

• If this module is imported with devices unavailable (e.g. get_cuda_test_devices() returning []), TestSpeculativeTunneling becomes an empty class that the @unittest.skipUnless(_cuda_available, …) wrapper still works for, but without any registered methods unittest just reports no tests (no visible skip). That's usually fine, but the class-level skipUnless is then purely decorative.
• Helper functions such as test_speculative_disabled_no_contacts are defined at module top level with a test_* prefix. If this file is ever discovered directly by a plain unittest/pytest loader (not via add_function_test), those top-level functions can be picked up as standalone tests and will fail because they take (test, device). Consider renaming the helpers (e.g. _test_… or _run_…) or moving them inside the class so they aren't accidentally collected.

Not a blocker — just worth tightening before de-drafting.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_speculative_contacts.py` around lines 467 - 503, Top-level
helper functions like test_speculative_disabled_no_contacts,
test_speculative_enabled_catches_fast_object, etc. are named with a test_*
prefix and can be discovered by unittest/pytest; also TestSpeculativeContacts
and TestSpeculativeTunneling are bare classes relying on add_function_test to
attach methods at import time which is fragile when device lists are empty. Fix
by renaming the helper functions to a non-discovered prefix (e.g. _test_ or
_run_) or move them inside the TestSpeculativeContacts/TestSpeculativeTunneling
classes, and add a minimal no-op test method (e.g. def test_placeholder(self):
pass) to each class so the `@unittest.skipUnless` on TestSpeculativeTunneling
still produces a visible skip when CUDA is unavailable; update usage of
add_function_test accordingly to reference the renamed functions if you choose
renaming.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes